Fix: Env-mediate expressions in run blocks - #148
Conversation
zizmor's auditor persona reported 7 template-injection findings in testing.yaml: GitHub expressions expanded directly inside 'run:' blocks, where expansion happens before the shell sees the script and a crafted value could inject commands. Two steps were affected, both validating action outputs: the static fixture check and the dynamic-version check. Their outputs now travel through 'env:'. The dynamic step's variables are prefixed DYN_ so the version string and the dynamic_version flag stay distinguishable. The org-wide zizmor workflow now fails a run on any finding at any level, so these informational findings block every new pull request in this repository. zizmor offers an auto-fix for this audit, but it is marked unsafe and on inspection it is: it rewrites the expression in place, leaving it inside the original single quotes, where the shell will not expand it. The original was correct because GitHub expanded the template before the shell ever saw it. These edits were therefore written by hand, with double quotes, and follow the env-before-run shape already used elsewhere in this repository. No behavioural change: the same values reach the same commands. Zizmor now reports no findings for the repository. Co-authored-by: Claude <claude@anthropic.com> Signed-off-by: Matthew Watkins <mwatkins@linuxfoundation.org>
There was a problem hiding this comment.
Pull request overview
This PR updates the CI workflow to prevent GitHub Actions expression expansion from being inlined into run: script source, by routing action outputs through step-level env: variables before use in Bash. This reduces the risk of template-injection findings (e.g., from zizmor) while preserving the existing validation behavior.
Changes:
- Move
steps.*.outputs.*references out ofrun:blocks and into stepenv:variables for the static output validation step. - Apply the same
env:mediation pattern to the dynamic-version validation step, usingDYN_-prefixed variable names to avoid ambiguity. - Adjust Bash quoting so env-provided values are treated as data (e.g.,
source="$VERSION_SOURCE").
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
zxiiro
left a comment
zxiiro
left a comment
There was a problem hiding this comment.
🤖 Auto-approved by agent: reviewed workflow/code change for security and CI/CD impact, found low risk. Mediates GitHub Actions expressions (step outputs) through env: vars instead of direct ${{ }} interpolation in shell run: steps, in test/CI workflow only; standard injection-hardening pattern, no new permissions or secrets.
Why
The org-wide zizmor workflow (
lfreleng-actions/.github/workflows/zizmor.yaml) was changed today:It runs
--persona auditor --min-severity informationaland now fails a run on any finding at any level. This repo carries 7 pre-existingtemplate-injectionfindings, so every new PR here is blocked until they're cleared — currently #147.What
.github/workflows/testing.yamlexpanded action outputs directly insiderun:blocks. GitHub expands${{ }}before the shell sees the script, so a crafted value becomes shell source text rather than data. Routing throughenv:makes the value opaque to the shell parser.Two steps were affected, both validating action outputs — the static-fixture check and the dynamic-version check:
The dynamic step's variables are prefixed
DYN_so the version string (DYN_VERSION) and the dynamic-versioning flag (DYN_IS_DYNAMIC) stay distinguishable — they wereversionanddynamiclocally, which would have collided as env names.Same shape
python-workflowsuses (see its "Fix: Env-mediate expressions in run blocks"), and theenv:-before-run:ordering already used elsewhere here.zizmor offers an auto-fix for this audit. It is marked unsafe, and on inspection it is — it rewrites the expression in place and leaves it inside the original single quotes:
The shell does not expand
${...}inside single quotes. The original was correct precisely because GitHub expanded the template before the shell ran; once the value moves toenv:, expansion becomes the shell's job and the quoting has to change with it. Four of the seven fixes here would have been silently broken by the auto-fix.Validation
zizmor --persona auditor --min-severity informational→ No findings to reportpre-commit run --all-files→ all hooks pass, includingyamllintandactionlintUnblocks #147.